feat: Send flow sub components #1906
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| @@ -101,6 +103,7 @@ export function AmountInput({ | |||
| '[appearance:textfield]', | |||
| '[&::-webkit-inner-spin-button]:m-0 [&::-webkit-inner-spin-button]:appearance-none', | |||
| '[&::-webkit-outer-spin-button]:m-0 [&::-webkit-outer-spin-button]:appearance-none', | |||
There was a problem hiding this comment.
nit: not related to your pr, but now that this is using a these styles shouldn't be needed
There was a problem hiding this comment.
could you clarify? what's being used that make which styles not needed?
| ); | ||
|
|
||
| return ( | ||
| <div className="grid w-full grid-cols-[2.5rem_1fr_auto] items-center gap-3"> |
There was a problem hiding this comment.
happy to add, but right now can't think of what people would want to change here. this is really just controlling the layout.
classNames.container applies to the outer element, and that can add background color, fonts, etc. what would be the purpose of adding an override here?
| <div className="text-right"> | ||
| {showAction ? ( | ||
| <div | ||
| role="button" |
There was a problem hiding this comment.
better to use a button than a div that thinks its a button
There was a problem hiding this comment.
this one can't be a button because sometimes the whole TokenBalance is a button and can't nest a button inside another button
There was a problem hiding this comment.
if possible, instead of nesting the buttons i'd use styles to overlay them. Its not an idea UX regardless, but at least it'd allow you to have two interactive elements working.
| showAction?: true; | ||
| actionText?: string; | ||
| onActionPress?: () => void; | ||
| } |
There was a problem hiding this comment.
if showAction, should actionText and onActionPress be required?
There was a problem hiding this comment.
refactored to this
| Exchange rate unavailable | ||
| </div> | ||
| ); | ||
| } |
There was a problem hiding this comment.
doesn't AmountInputTypeSwitch already handle loading, etc, why is this wrapped necessary?
There was a problem hiding this comment.
AmountInputTypeSwitch's method for handling loading is to show an infinite blank Skeleton. I don't want that behavior.
Better to add an override to AmountInputTypeSwitch, which will default to current behavior, but into which I can pass my div?
| '_blank', | ||
| 'noopener,noreferrer', | ||
| ); | ||
| } |
There was a problem hiding this comment.
how is this different from the default success handler?
There was a problem hiding this comment.
after it's clicked it takes an action (setting showSend to false)
| } | ||
|
|
||
| return 'Continue'; | ||
| } |
There was a problem hiding this comment.
if this is only used with a single button, may make sense to keep this logic with the button
There was a problem hiding this comment.
yeah no intention to reuse this, i extracted for readability, but can add back if you prefer
| return { | ||
| display: input, | ||
| value: selectedAddress, | ||
| }; |
There was a problem hiding this comment.
this may also be better suited to live where its used, could leave display '' if input is an address and render getSlicedAddress. hard to tell in this limited context.
| @@ -16,6 +16,7 @@ type AmountInputProps = { | |||
| setCryptoAmount: (value: string) => void; | |||
| exchangeRate: string; | |||
| className?: string; | |||
| textClassName?: string; | |||
There was a problem hiding this comment.
we could use an object structure here with classNames like dan suggested in quality TDD
There was a problem hiding this comment.
will that be a breaking change for FundCard though?
There was a problem hiding this comment.
oh yeah good point i guess we can revisit here and i saw you updated to classNames in other component 👍
src/wallet/components/wallet-advanced-send/components/SendAmountInputTypeSwitch.tsx
Outdated
Show resolved
Hide resolved
src/wallet/components/wallet-advanced-send/components/SendFundWallet.tsx
Outdated
Show resolved
Hide resolved
src/wallet/components/wallet-advanced-send/components/SendTokenSelector.tsx
Show resolved
Hide resolved
| exchangeRateLoading, | ||
| className, | ||
| textClassName, | ||
| }: SendAmountInputProps) { |
There was a problem hiding this comment.
Should SendAmountInputProps live in this file? I think we're defaulting to that now if this is its only use
There was a problem hiding this comment.
ah okay yeah that should be fine, i don't think it really needs to be reused anywhere, i was just colo'ing all types in the type file. will move back
There was a problem hiding this comment.
hmm actually this is reused in SendAmountInputTypeSwitch, so I think it should stay in the types file
brendan-defi
force-pushed
the
feat/send-sub-components
branch
from
eaa3510 to
1aa46ca
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
What changed? Why?
Notes to reviewers
How has it been tested?